Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-1765 Update CrashContext with necessary info for handling App Launch crashes - part 3 #690

Conversation

ncreated
Copy link
Member

What and why?

📦 This PR prepares CrashContext to handle crashes during application launch. This is necessary to decide:

  • if the crash should be send in "ApplicationLaunch" view (when app crashed during foreground launch);
  • if the crash should be send in "Background" view (when app crashed during background launch);
  • if the crash should be dropped (e.g. when app crashed during background launch, but Background Events Tracking is disabled OR when crashed session was rejected by sampler).

How?

First, I isolated the code deciding on "ApplicationLaunch" x "Background" x "no" view to RUMOffViewEventsHandlingRule. This logic is quite complex but very important (for customer billing 💸) so it's definitely worth having it separate as it will be used by both RUM and Crash Reporting to take similar decision on handling events or crashes while no active view:

application-launch-events

Second, I updated CrashContext with all values necessary for making the decision ☝️ in RUMOffViewEventsHandlingRule. This way, when the app restarts upon the crash, we will be able to run exactly the same logic and decide if the crash should be sent in "ApplicationLaunch" view, "Background" view or if it should be dropped (e.g. due to sampling).

Crash handling upon app restart will be delivered in separate, final PR to this topic.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

… foreground / background state

so it can be accessed in restarted session, when deciding on how to upload crash report.
@@ -75,91 +75,109 @@ class RUMSessionScopeTests: XCTestCase {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I updated RUMSessionScope tests to transitively cover all cases in RUMOffViewEventsHandlingRule used internally. I took this approach instead of abstracting RUMOffViewEventsHandlingRule through interface and mocking it for tests. I wanted to optimise code readability in RUMSessionScope, even if the cost was more effort in fuzzy testing.


// MARK: Integration with Crash Context

func testWhenViewIsStarted_thenItUpdatesLastRUMViewEventInCrashContext() throws {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this missing test, because now RUMWithCrashContextIntegration is injected through DI and we're able to configure the mock.

@ncreated ncreated marked this pull request as ready for review December 16, 2021 16:40
@ncreated ncreated requested a review from a team as a code owner December 16, 2021 16:40
@ncreated ncreated self-assigned this Dec 17, 2021
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear! The doc and the separation of concern applied in RUMOffViewEventsHandlingRule help understanding the complexity of this feature.

I'm wondering about the rules applied when session state is nil, I wonder if we shouldn't drop the event instead.

/// - isAppInForeground: if the app is in foreground
/// - isBETEnabled: if Background Events Tracking feature is enabled in SDK configuration
init(
sessionState: RUMSessionState?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a event cannot be sent outside of a session, shouldn't we apply this rule only if we have a session state? this would mean making the state non-optional.
And if it's more convenient to keep it optional, why not dropping the event if the session state is nil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (sessionState: nil) is used later in #695 to decide on handling crashes which occurred as the very first event in the app. For example, in case of:

Datadog.initialize(
   ...
)

fatalError("crash")

we don't have RUMSession yet created, so its persisted (through CrashContext) state will be nil.

I've spent a while trying to model the minimal information we need to persist in CrashContext for differentiating crashes during app launch in foreground vs background and resulted with this RUMSessionState. If you can find a better model, I'd be happy to adopt it and simplify things 🙂 🙌! I'm all for consistency here, making things optional just for convenience might be error-prone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the model is great and simplifies the rule definition!

In the next PRs, will the RUMSessionState.sessionUUID will be leveraged somehow for other purposes than checking if it's sampled ou?
I'm wondering if we could do something like:

internal struct RUMSessionState: Equatable, Codable {
    let isSampled: Bool
    let isInitialSession: Bool
    let hasTrackedAnyView: Bool

    static let `default` = RUMSessionState(isSampled: false, isInitialSession: true, hasTrackedAnyView: false)
}

This way, we could make the state non-optional and clarify the statements in this init?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next PRs, will the RUMSessionState.sessionUUID will be leveraged somehow for other purposes than checking if it's sampled out?

Now I think that in #695 I forgot to handle the case which I spotted when designing RUMSessionState 😰. Indeed, it may look now that sessionUUID is not necessary and we don't use it in #695 now.

The plan was to use sessionUUID in this scenario:

  • 1/ SDK has Background Events Tracking enabled and views instrumentation turned ON;
  • 2/ It tracks some views, leading to updating lastRUMViewEvent in CrashContext;
  • 3/ Then the app is sent to background (so it stops the recent view);
  • 4/ No other event occurs in the background except the crash.

In such case, because there is no active view (3) and the app crashed in background (4) we should send the crash in "Background" view (1). No additional sampling should be considered and both crash and its "Background" view should be sent to the sessionUUID used for tracking other views in this session (2).

That said: no, isSampled: Bool wouldn't be enough for RUMSessionState - we also need to know its uuid for above case ☝️. Knowing that .nullUUID means "not sampled", it also simplifies few more edge cases.

I will update next PR (#695) with necessary logic for handling this case 👍. Otherwise the RUMSessionState persistence in CrashContext doesn't make sense at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense then 👍

Base automatically changed from ncreated/RUMM-1765-do-not-start-application-launch-view-when-app-in-background to ncreated/RUMM-1765-app-launch-events-tracking December 20, 2021 17:46
@ncreated ncreated requested a review from maxep December 20, 2021 18:02
@ncreated ncreated merged commit bb4732b into ncreated/RUMM-1765-app-launch-events-tracking Dec 21, 2021
@ncreated ncreated deleted the ncreated/RUMM-1765-collect-app-launch-crashes branch December 21, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants